With the assertions in place now, these are no longer a safety issue but rather just panic scenarios, so I'd change the heading here from Safety to Panics and update the wording of the section.

I'm not sure how to express it. Could you provide some content?

Can you give it a try first? I can help if/when needed. Just try to keep the changes to minimum and focus on adapting the wording.

This is still not done.

The cllipy requires a Safety section, so I added the content under it.
Maybe we have to make this function safe to change it to Panics

The cllipy requires a Safety section, so I added the content under it. Maybe we have to make this function safe to change it to Panics

If there is no safety issues anymore, yeah I think we should do that.

Still, please bring back the changes I requested. If there are no safety issues anymore but for some reason we still want this to be declared unsafe, we should just say so in the Safety section (separate from Panics): Currently this function is completely safe.

@sgued What do you think of removing unsafe from the function? I think it's not considered a breaking change.

The init function takes pointers as arguments so it still needs to be unsafe because safe code could generate bogus pointers.

Ah right. I had assumed that @JalonWong did his homework but I guess not. :) So @JalonWong we need both Safety and Panics sections then.

@zeenix Yes, I noticed that the arguments need unsafe. I wonder if we should continue to modify it? Or what content should be written in the Safety section if we keep both Safety and Panics sections?

So my question was that why do we need a separate boolean field with it's own mutex when could use core::cell::OnceCell? We can bump the MSRV if that's an issue here.

I use Mutex because it's a hint that we should use it in a critical section. Since the Mutex of critical_section is basically an UnsafeCell, I think it's not bad.
And because Sync is not implemented for OnceCell, using it directly might not be safe enough.

I am not talking about use of Mutex. I'm questing the need for the addition of an extra field. Why can't heap be changed to Mutex<OnceCell<LLHpeap>>? If it can be, then we don't need another field here.

It sounds good, and I have tried it. It turns out that OnceCell needs a mutable reference to call get_mut(), but the Mutex of critical_section can not provide it. It seems that the RefCell is necessary here.

Unless I do Mutex<RefCell<OnceCell<LLHeap>>> or Mutex<OnceCell<RefCell<LLHeap>>>. Are they better?

It sounds good, and I have tried it. It turns out that OnceCell needs a mutable reference to call get_mut(), but the Mutex of critical_section can not provide it. It seems that the RefCell is necessary here.

Right, I keep forgetting this. It's just weird for something to be called a "Mutex" can it not providing interior mutability. I think it's a big naming failure but naming things is hard. :)

Unless I do Mutex<RefCell<OnceCell<LLHeap>>> or Mutex<OnceCell<RefCell<LLHeap>>>. Are they better?

Not ideal but IMO better than having a separate field.

I don't like adding one more branch to each allocation.

You mean indirection (cause I don't see any branches because combinators are being used)? I don't love it either but I think the code is more robust if the fate of a field is not decided by another (especially if the fate involves safety). This is one of the reasons I fell in love with Rust: in C and C++ (and other languages) locks guarding a state are separate and therefore vulnerable to programmers making a mistake. In Rust, we have the guarded state enclosed in the locks. I think this is a very similar case here.

As said in #103 (comment) this does not fully solve the safety issue.

I'm confused, that comment just says that the function still need to be marked as unsafe. That's not too relevant to the overall picture IMO. 🤔

You mean indirection (cause I don't see any branches because combinators are being used)? I don't love it either but I think the code is more robust if the fate of a field is not decided by another (especially if the fate involves safety).

The allocation functions now have an additional unwrap. As seen in rust-embedded/heapless#598 that can seriously impact performance.

This is also a breaking change because now allocating with an empty heap panics, while before it just behaved as out of memory.

I'm confused, that comment just says that the function still need to be marked as unsafe. That's not too relevant to the overall picture IMO. 🤔

My point is that sacrificing performance for a bit of safety but not full safety is not that clear-cut for me. Especially for something like this that is pretty easy to use safely in the embedded use case, where performance can be important.
If the cost of the check was only in the init function it would be fine, but here it's not, it affects every allocation.

You mean indirection (cause I don't see any branches because combinators are being used)? I don't love it either but I think the code is more robust if the fate of a field is not decided by another (especially if the fate involves safety).

The allocation functions now have an additional unwrap. As seen in rust-embedded/heapless#598 that can seriously impact performance.

Right, I didn't think of performance penalty. Now that you have mentioned it, it is indeed a good argument. Sorry @JalonWong for making you go down that road. Let's go with your previous approach but first let's make sure both I and @sgued agree on that as well.

(BTW I think you meant to link to rust-embedded/heapless#598).

This is also a breaking change because now allocating with an empty heap panics, while before it just behaved as out of memory.

The docs say "Obey these or Bad Stuff will happen" about "n == 0" so we promised them more pain that panic brings so IMO this makes it the opposite of a breaking change. :)

My point is that sacrificing performance for a bit of safety but not full safety is not that clear-cut for me.

I don't think you presented this argument before but now that you have, I agree. :)

Do you agree with @JalonWong's previous approach (with two fields)?

The docs say "Obey these or Bad Stuff will happen" about "n == 0" so we promised them more pain that panic brings so IMO this makes it the opposite of a breaking change. :)

I meant allocating without ever calling init.
This can be done with safe code. But yes we can argue it's not really an issue.

Do you agree with @JalonWong's previous approach (with two fields)?

I can be ok with that. Just a bool in the same RefCell as the heap RefCell that is checked on init.

@zeenix I agree with @sgued at this point, only checking in init gets better performance.
I'll roll back to previous version.